-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Import PaginationNavigation
from client as Pagination
#1817
Conversation
@@ -0,0 +1,148 @@ | |||
import classnames from 'classnames'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unchanged from PaginationNavigation
in the client aside from the rename and a few minor JSDoc additions.
@@ -0,0 +1,132 @@ | |||
import { checkAccessibility, mount } from '@hypothesis/frontend-testing'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unchanged from PaginationNavigation
in the client aside from removing a few blank lines and unnecessary act
calls.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1817 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 68 70 +2
Lines 1234 1273 +39
Branches 468 478 +10
=========================================
+ Hits 1234 1273 +39 ☔ View full report in Codecov by Sentry. |
@@ -0,0 +1,73 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copied unchanged from the client repo. Same for the tests.
* pageNumberOptions(3, 10, 5) => [1, 2, 3, 4, null, 10] | ||
* pageNumberOptions(6, 10, 5) => [1, null, 5, 6, 7, null, 10] | ||
* pageNumberOptions(9, 10, 5) => [1, null, 7, 8, 9, 10] | ||
* pageNumberOptions(2, 3, 5) => [1, 2, 3] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a flaw in the current design in that the number of entries in the array changes depending on whether any are elided. This tends to cause the width to change. Ideally we want the overall width of the control to stay the same and for the prev/next buttons to stay in a fixed position. This would be easier if there were always the same number of items between the prev/next buttons and each had the same width.
99b3d43
to
7e07203
Compare
variant="custom" | ||
/> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some visual issues here: the Prev/Next buttons have a different height than the others, and un-pressed buttons have a grey background that matches the background color of the Notebook, instead of no background. I will address this as part of a design update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
> | ||
<Library.Section> | ||
<Library.Pattern> | ||
<Library.Usage componentName="Pagination" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
componentName
is deprecated in favor of symbolName
.
<Library.Usage componentName="Pagination" /> | |
<Library.Usage symbolName="Pagination" /> |
src/index.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also need to re-export PaginationProps
here.
Import the `PaginationNavigation` component from the hypothesis/client repository and rename it to the more succinct `Pagination`.
7e07203
to
7e21140
Compare
Import the
PaginationNavigation
component from the hypothesis/client repository and rename it to the more succinctPagination
.Go to http://localhost:4001/navigation-pagination to see it in action.
In this initial import the design of the control is unchanged from how it appears in the client's Notebook. In subsequent PRs I will update the design to align with the mocks.